Make sure the HTTP call is never made inside a database transaction. #1099

GeoffreyHuck · 2024-07-08T12:06:39Z

Because we want to have fast transaction.

Now, we call the endpoint at the end of the current transaction.

Tricky part: How to pass the config endpoint to the `DataStore`

That was the trickiest part.

The first question is: Do DataStore need to know about how to schedule the propagation? I choose that the answer was yes because ~~it was easier~~ the fact we do it after a transaction means it makes sense to be aware of transactions.

Also, InTransaction is called in many places, and it doesn't make sense at all to pass the endpoint as a parameter to this function, because most transactions are not about propagations at all.

What was done: the request Context was already passed to the DataStore, but it wasn't used. So the endpoint is now stored in the request Context, in a middleware, and retrieved from the DataStore. It's the easiest way I found.

Alternatives

Make the configuration available globally. It's always tempting, but if we start like this we might end up with more and more globals.
Have another method called InTransactionWithPropagation with the endpoint as parameter, that does the same as InTransaction but with a propagation. The downside is that we would have to know which one to use at the start of the transaction. It seems to me that the propagation is a notion that appears at a lower level than the service.

Make sure the propagation types are unique when we call the endpoint

We also make the types of propagation, sent as a parameter when we call the propagation endpoint, unique. We don't have to, it would work without it, but it seems cleaner.

Review

Easier to review commit by commit. Details in the commit messages.

…because it calls an endpoint. Signed-off-by: Geoffrey Huck <[email protected]>

…ntext in the DataStore. We need the endpoint to call the async propagation. And it has to be from DataStore because we want to specifically do it after the current transaction. Signed-off-by: Geoffrey Huck <[email protected]>

…lic. Signed-off-by: Geoffrey Huck <[email protected]>

…t does. Move it in the `database` package. Because otherwise we have a cyclic dependence error when calling it from the `database` package (database -> service -> auth -> database). This might be solvable in another way though, but it would require refactoring. Signed-off-by: Geoffrey Huck <[email protected]>

…he calls in services. Signed-off-by: Geoffrey Huck <[email protected]>

…in the slice. It's not a requirement, it would work without it, but it feels better. Also, the endpoint is called with the types, so it avoids redundancy there too. Signed-off-by: Geoffrey Huck <[email protected]>

Signed-off-by: Geoffrey Huck <[email protected]>

…enAndRunListeners`. It makes more sense because the work that needs a propagation is done there already. Signed-off-by: Geoffrey Huck <[email protected]>

@see

…guration in the `request Context`. @see #1099 Signed-off-by: Geoffrey Huck <[email protected]>

codecov · 2024-07-08T12:34:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4d1012c) to head (dd140bf).
Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #1099   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          239       239           
  Lines        14462     14480   +18     
=========================================
+ Hits         14462     14480   +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Geoffrey Huck <[email protected]>

…chedule it asynchronously. Because we expect the database to be propagated after the command ends. Signed-off-by: Geoffrey Huck <[email protected]>

…s is intended because in this situation, it shouldn't have a value. Signed-off-by: Geoffrey Huck <[email protected]>

… propagation. In case no endpoint is defined, like for the tests, it was run after, because in this case the propagation is run in sync. When that happens, some tests don't pass. Signed-off-by: Geoffrey Huck <[email protected]>

@see

…l propagations when one is done, there is no distinction between with and without results propagation anymore. @see #1100 Signed-off-by: Geoffrey Huck <[email protected]>

@see

…l propagations when one is done, the permissions propagation is always done (because the results propagation is always done). @see #1100 Signed-off-by: Geoffrey Huck <[email protected]>

smadbe · 2024-07-09T15:33:36Z

I would prefer that @zenovich reviews this one.

zenovich · 2024-07-10T05:33:26Z

The first question is: Do DataStore need to know about how to schedule the propagation? I choose that the answer was yes because it was easier the fact we do it after a transaction means it makes sense to be aware of transactions.

I still believe that the answer is "no" as it was discussed days before on Slack. The reason is that DataStore is just a clever DB interface decorator. But probably we could rethink the meaning of DataStore.

And actually I need to study this a bit more from scratch, as, from my perspective (from what it looked like two years ago), mutating operations on the DB resulted in setting the DB into inconsistent state (because of the operations themselves and also because of MySQL triggers), the propagations fixed that by bringing the DB into the consistent state again. That was the reason of calling the propagations in the same transaction with the mutating operations.

So, first I would like to study the current situation.

@smadbe, is it an urgent task?

zenovich · 2024-07-15T00:05:38Z

The main thing:
If we schedule a propagation outside of a transaction, we cannot guarantee that data modifications already committed to the DB will be followed by a corresponding scheduled propagation. Indeed, let's consider a situation where, for instance, an AWS Lamba function processing a user request gets killed because of a timeout right after it has committed the data modifications to the DB, but before it has called another AWS Lambda function to trigger the propagation.

Such situations are really dangerous and hard to debug. They will definitely lead to production failures and DB inconsistency issues.

I would think 100500 times before doing this and finally would find another approach.

zenovich · 2024-07-15T00:21:02Z

app/database/data_store.go

+	s.mustBeInTransaction()
+
+	triggersToRun := s.DB.ctx.Value(triggersContextKey).(*awaitingTriggers)
+	triggersToRun.SchedulePropagationTypes = utils.UniqueStrings(append(triggersToRun.SchedulePropagationTypes, types...))


If the order of types doesn't matter, it would be much cleaner to store map[string]struct{} in triggersToRun.SchedulePropagationTypes instead of creating utils.UniqueStrings() and tests for it. Also, the map works faster. But a struct containing all the possible values as boolean fields (like 'awaitingTriggers') would be even better than the map: it would store only several booleans while the map stores strings. Also, the struct provides immediate access to its fields, while the map uses hashing. Also, with the struct, it is not possible to mistype a value which is an often issue with maps/slices.

If the order of types matters, UniqueString() breaks it anyway since maps don't preserve the order of keys.

zenovich · 2024-07-15T00:21:34Z

app/database/data_store.go

@@ -262,10 +279,6 @@ func (s *DataStore) WithForeignKeyChecksDisabled(blockFunc func(*DataStore) erro
 	})
 }

-func (s *DataStore) IsInTransaction() bool {


It was a nice useful method.

zenovich · 2024-07-15T00:24:42Z

app/database/propagation.go

+
+			return nil
+		})
+		mustNotBeError(err)


Public methods cannot return errors via panic, panic/recover pattern should be used only within a package (see https://go.dev/doc/effective_go#recover). Also, it is an "architecture decision" made in March 2019, which we have always been following since then.

zenovich · 2024-07-15T00:29:08Z

app/database/propagation.go

 // If endpoint is an empty string, it will be done synchronously.
-func SchedulePropagation(store *database.DataStore, endpoint string, types []string) {
+func StartAsyncPropagation(store *DataStore, endpoint string, types []string) {


I would move this method into app/service. It can be called right from the endpoint handler in case when the async propagation is chosen. The database package is for database-related things only.

zenovich · 2024-07-15T00:35:13Z

app/utils/string_test.go

@@ -1,6 +1,8 @@
 package utils


A package should not be called 'utils' as all the packages should have meaningful names. 'utils' is a classic example of bad package names. We should do something with it (see https://go.dev/blog/package-names)

zenovich

All the above

Make all propagations async

GeoffreyHuck added 9 commits June 16, 2024 08:59

Make sure SchedulePropagation is never called inside a transaction …

1e4c869

…because it calls an endpoint. Signed-off-by: Geoffrey Huck <[email protected]>

This constant is only used in this file, so it doesn't have to be pub…

2b87da4

…lic. Signed-off-by: Geoffrey Huck <[email protected]>

The propagation is now called after the current transaction. Update t…

3e53ee0

…he calls in services. Signed-off-by: Geoffrey Huck <[email protected]>

Fix lint order of imports.

d4ecc20

Signed-off-by: Geoffrey Huck <[email protected]>

Fix lint cyclomatic complexity: schedule propagation in `updateChildr…

469b28f

…enAndRunListeners`. It makes more sense because the work that needs a propagation is done there already. Signed-off-by: Geoffrey Huck <[email protected]>

Update test: we have one more middleware to inject the endpoint confi…

354d2e5

…guration in the `request Context`. @see #1099 Signed-off-by: Geoffrey Huck <[email protected]>

GeoffreyHuck changed the title ~~WIP: Make sure the HTTP call is never made inside a database transaction.~~ Make sure the HTTP call is never made inside a database transaction. Jul 8, 2024

Cleanup: function not used anymore.

dd140bf

Signed-off-by: Geoffrey Huck <[email protected]>

GeoffreyHuck requested a review from smadbe July 8, 2024 12:52

GeoffreyHuck added 7 commits July 8, 2024 16:58

All the propagations are now async.

f74233d

Signed-off-by: Geoffrey Huck <[email protected]>

Fix typo.

46b8a27

Signed-off-by: Geoffrey Huck <[email protected]>

In the migrate command, we need to call the propagation now and not s…

3c34f0e

…chedule it asynchronously. Because we expect the database to be propagated after the command ends. Signed-off-by: Geoffrey Huck <[email protected]>

In the unit tests, the propagation endpoint hasn't been injected. Thi…

2626486

…s is intended because in this situation, it shouldn't have a value. Signed-off-by: Geoffrey Huck <[email protected]>

Fix test. Since we run all propagations async, and the system does al…

bd079a0

…l propagations when one is done, there is no distinction between with and without results propagation anymore. @see #1100 Signed-off-by: Geoffrey Huck <[email protected]>

Fix test. Since we run all propagations async, and the system does al…

27d5a62

…l propagations when one is done, the permissions propagation is always done (because the results propagation is always done). @see #1100 Signed-off-by: Geoffrey Huck <[email protected]>

smadbe requested review from zenovich and removed request for smadbe July 9, 2024 15:33

zenovich reviewed Jul 15, 2024

View reviewed changes

zenovich requested changes Jul 15, 2024

View reviewed changes

zenovich assigned GeoffreyHuck Jul 15, 2024

Merge pull request #1100 from France-ioi/1047

0415b3b

Make all propagations async

GeoffreyHuck mentioned this pull request Jul 24, 2024

Potential inconsistances with propagation outside of transaction #1129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure the HTTP call is never made inside a database transaction. #1099

Make sure the HTTP call is never made inside a database transaction. #1099

GeoffreyHuck commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

smadbe commented Jul 9, 2024

zenovich commented Jul 10, 2024

zenovich commented Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024

zenovich Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024

zenovich Jul 15, 2024 •

edited

Loading

zenovich left a comment

Make sure the HTTP call is never made inside a database transaction. #1099

Are you sure you want to change the base?

Make sure the HTTP call is never made inside a database transaction. #1099

Conversation

GeoffreyHuck commented Jul 8, 2024 • edited Loading

Tricky part: How to pass the config endpoint to the DataStore

Alternatives

Make sure the propagation types are unique when we call the endpoint

Review

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

smadbe commented Jul 9, 2024

zenovich commented Jul 10, 2024

zenovich commented Jul 15, 2024 • edited Loading

zenovich Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

zenovich Jul 15, 2024

Choose a reason for hiding this comment

zenovich Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

zenovich Jul 15, 2024

Choose a reason for hiding this comment

zenovich Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

zenovich left a comment

Choose a reason for hiding this comment

GeoffreyHuck commented Jul 8, 2024 •

edited

Loading

Tricky part: How to pass the config endpoint to the `DataStore`

codecov bot commented Jul 8, 2024 •

edited

Loading

zenovich commented Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024 •

edited

Loading

zenovich Jul 15, 2024 •

edited

Loading